Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Stop() to Meter #197

Merged
merged 1 commit into from
Nov 28, 2017
Merged

add Stop() to Meter #197

merged 1 commit into from
Nov 28, 2017

Conversation

vetinari
Copy link
Contributor

@vetinari vetinari commented Feb 9, 2017

this also removes the meter from the tickers.

@imkira
Copy link

imkira commented Apr 13, 2017

@vetinari I am thinking, shouldn't we add Stop to Timer too?

meter: NewMeter(),

@imkira imkira mentioned this pull request Apr 13, 2017
@@ -140,6 +158,9 @@ func (m *StandardMeter) Count() int64 {
func (m *StandardMeter) Mark(n int64) {
m.lock.Lock()
defer m.lock.Unlock()
if m.stopped {
panic("Mark called on a stopped Meter")
Copy link

@jpudney jpudney Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should really return error instead of panicking?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and change the interface for that? That's like the Mark() on a snapshot which also panics

Copy link

@imkira imkira May 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up until now there is no panic on anything else except snapshots and the like, which are unconditional panics. If we do this here, extreme caution must be taken while doing unregister+Mark concurrently. Being able to safely ignore (no-op) this would be better IMHO

@@ -224,6 +246,12 @@ func (ma *meterArbiter) tick() {
}
}

// should only be called with Lock() held
func (ma *meterArbiter) newID() int64 {
ma.id++
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone, somewhere will overflow this

Copy link

@imkira imkira Apr 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also made me a bit concerned.
The only reason why we want a map is to have constant-time deletion, so we don't actually need the concept of "IDs" for metrics.
Couldn't we register the meter in the arbiter's map having both key and value as the pointer to itself (avoid ID generation)? We could also add a check to stopped to make sure it gets done only once.

@PSanetra
Copy link

Is there an ETA for merging this PR?

@@ -208,11 +229,12 @@ func (m *StandardMeter) tick() {
type meterArbiter struct {
sync.RWMutex
started bool
meters []*StandardMeter
meters map[int64]*StandardMeter
Copy link
Contributor

@slaunay slaunay Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using a set of *StandardMeter aka map[*StandardMeter]struct{} to avoid id collision/overflow yet benefit from fast and easy deletion?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that would be a better solution than adding the ID generation

@@ -15,6 +15,7 @@ type Meter interface {
Rate15() float64
RateMean() float64
Snapshot() Meter
Stop()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add the same method on Timer as they rely on a Meter under the hood.

I am even wondering if having an interface Stoppable implemented by Timer and Meter and invoking Stop in Registry.Unregister(string) when the metric implements such an interface is the proper way to clean those metrics that can leak (potentially supporting similar behavior for custom metrics).

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrasing just so I understand: you're suggesting we add the interface and implementations, then leave it up to the user to call Unregister when a metric has outlived its usefulness, correct? I suppose there's not really a magical way we can know when it's safe to do so internally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mihasya Absolutely.

Unregister (and UnregisterAll by extension) would check if the underlying interface{} metric implements Stoppable and call Stop() as like you said there is no way to know when those metrics are not used anymore.

@imkira
Copy link

imkira commented Jul 9, 2017

#206 actually improves on this one.
However, I think we should do as @slaunay said and avoid mapping of IDs to objects.

@lday0321
Copy link

Hope to fix this leak problem

@mihasya
Copy link
Collaborator

mihasya commented Aug 31, 2017

Sorry, I've been a horrendous maintainer, but this bug definitely needs a fix.

I've made some comments on this that also apply to #206. I am in favor of both of @slaunay's comments - if we can use the pointer as the hash key and call Stop on any metric that type-asserts to a to-be-added Stoppable interface inside Registry.Unregister, I will happily merge.

@inooka-shiroyuki
Copy link
Contributor

@mihasya
I merged the PR from @slaunay to #206.
I think it satisfies your comments above.
Please confirm it.

@slaunay
Copy link
Contributor

slaunay commented Sep 20, 2017

@mihasya What do you think of #206 recent changes?

@slaunay
Copy link
Contributor

slaunay commented Oct 23, 2017

@mihasya Any chance you can take a look at those changes as it is impacting quite a few downstream projects?

@mihasya mihasya merged commit c83b0ea into rcrowley:master Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants